[PE-210] feat: editor performance #6269
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request focuses on updating dependencies and refactoring the editor-related code in the Plane project. The changes primarily involve upgrading package versions across multiple files, including Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Palanikannan1437
force-pushed
the
fix/upgrade-editor
branch
from
cd8212f to
a207653
Compare
Palanikannan1437
force-pushed
the
fix/upgrade-editor
branch
2 times, most recently
from
6985452 to
bbe378e
Compare
Palanikannan1437
force-pushed
the
fix/upgrade-editor
branch
from
bbe378e to
5ff8514
Compare
Palanikannan1437
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/editor/src/core/helpers/insert-content-at-cursor-position.ts (1)
14-15: Consider using early return pattern for better readability.The position calculation could be simplified using early returns.
- const docSize = editor.state.doc.content.size; - const safePosition = Math.max(0, Math.min(editor.state.selection.anchor, docSize)); + if (!editor.state.selection.anchor) return; + const safePosition = Math.min(editor.state.selection.anchor, editor.state.doc.content.size);packages/editor/src/core/hooks/use-editor.ts (2)
117-121: Improve error boundary for selection handling.Consider wrapping the selection handling in a separate try-catch block to prevent the entire content sync from failing.
try { editor.commands.setContent(value, false, { preserveWhitespace: "full" }); + } catch (error) { + console.error("Error syncing editor content:", error); + } + try { if (editor.state.selection) { const docLength = editor.state.doc.content.size; const relativePosition = Math.min(editor.state.selection.from, docLength - 1); editor.commands.setTextSelection(relativePosition); } } catch (error) { - console.error("Error syncing editor content with external value:", error); + console.error("Error restoring selection:", error); }
Line range hint
236-254: Optimize text selection performance.The
getSelectedTextimplementation could be optimized by reducing DOM operations.if (empty) return null; const nodesArray: string[] = []; + const tempDiv = document.createElement("div"); state.doc.nodesBetween(from, to, (node, _pos, parent) => { if (parent === state.doc && editor) { const serializer = DOMSerializer.fromSchema(editor.schema); const dom = serializer.serializeNode(node); - const tempDiv = document.createElement("div"); tempDiv.appendChild(dom); nodesArray.push(tempDiv.innerHTML); + tempDiv.innerHTML = ''; } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
live/package.json(2 hunks)packages/editor/package.json(3 hunks)packages/editor/src/core/components/menus/menu-items.ts(1 hunks)packages/editor/src/core/helpers/insert-content-at-cursor-position.ts(1 hunks)packages/editor/src/core/hooks/use-editor.ts(10 hunks)packages/editor/src/core/hooks/use-read-only-editor.ts(5 hunks)packages/editor/src/core/types/editor.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/core/types/editor.ts
🔇 Additional comments (9)
packages/editor/src/core/helpers/insert-content-at-cursor-position.ts (1)
3-4: LGTM! Improved function signature.The change from
MutableRefObject<Editor | null>to directEditorinstance reduces indirection and simplifies the code.packages/editor/src/core/hooks/use-read-only-editor.ts (2)
43-46: Performance optimization through render control.Good addition of
immediatelyRenderandshouldRerenderOnTransactionflags to optimize rendering performance.
78-81: Verify the impact of skipImageDeletion meta flag.The
skipImageDeletionmeta flag is used in clearEditor. Ensure this doesn't affect image cleanup.Also applies to: 84-85
✅ Verification successful
The skipImageDeletion meta flag implementation is correct
The flag is properly used in the TrackImageDeletionPlugin to skip cleanup only during programmatic content clearing, while maintaining normal image deletion functionality for user actions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for skipImageDeletion usage and related image cleanup logic rg -A 5 "skipImageDeletion|imageCleanup"Length of output: 1937
Script:
#!/bin/bash # Search for image-related tests and handlers rg -g "*.test.*" -A 5 "image|Image" # Search for additional image-related logic in plugins ast-grep --pattern 'class $_ extends Node { $$$ get name() { return "image" } $$$ }'Length of output: 285
Script:
#!/bin/bash # Search for test files fd -e test.ts -e test.tsx -x rg -l "image|Image" {} # Search for image node and deletion logic rg -g "*.ts" -A 10 "class.*Image|deleteImage|onDelete.*image" # Search for image-related extensions or plugins rg -g "*.ts" "addImage|createImage|uploadImage"Length of output: 4970
packages/editor/src/core/components/menus/menu-items.ts (1)
205-205: LGTM! Simplified selection handling.Good change to use editor's selection state directly instead of maintaining a separate saved selection.
packages/editor/src/core/hooks/use-editor.ts (1)
77-78: Performance optimization through render control.Good addition of render control flags. However, verify if setting both flags to false doesn't affect editor responsiveness.
live/package.json (2)
43-43: Verify y-prosemirror and yjs compatibilityThe updates to y-prosemirror (1.2.9 → 1.2.15) and yjs (13.6.14 → 13.6.20) appear to be patch versions, but we should verify compatibility between these interdependent packages.
Also applies to: 45-45
✅ Verification successful
✓ Compatibility verified between y-prosemirror 1.2.15 and yjs 13.6.20
Both updates contain only bug fixes and improvements with no breaking changes or compatibility issues reported in their release notes. The changes enhance stability and fix various edge cases while maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in release notes echo "Checking y-prosemirror changelog..." gh api repos/yjs/y-prosemirror/releases | jq -r ".[].body" echo "Checking yjs changelog..." gh api repos/yjs/yjs/releases | jq -r ".[].body"Length of output: 20227
19-22: Verify @hocuspocus packages compatibilityThe @hocuspocus packages have been updated to v2.15.0. While this appears to be a minor version bump, we should verify there are no breaking changes.
✅ Verification successful
@hocuspocus package updates appear safe
The minor version bump (2.15.0) with consistent versioning across all @hocuspocus packages, combined with standard API usage patterns in the codebase, suggests this update is safe. The codebase uses well-established APIs that are unlikely to change in minor versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in release notes for pkg in extension-database extension-logger extension-redis server; do echo "Checking @hocuspocus/$pkg changelog..." gh api repos/ueberdosis/hocuspocus/releases | jq -r ".[] | select(.tag_name | contains(\"$pkg\")) | .body" doneLength of output: 1180
Script:
#!/bin/bash # Check peer dependencies and usage patterns echo "Checking package dependencies..." fd package.json --exec cat {} \; | jq -r '.peerDependencies | select(. != null)' echo -e "\nChecking @hocuspocus usage patterns..." rg "@hocuspocus" --type ts --type js -A 2 -B 2Length of output: 2690
packages/editor/package.json (2)
15-15: Verify impact of removing module exportsThe removal of the "module" field from exports could affect bundlers' module resolution. Please ensure this change doesn't impact build optimization.
Consider testing the build process to verify:
- Bundle size hasn't increased
- Tree-shaking still works effectively
- No resolution errors in dependent packages
Also applies to: 20-20
37-37: Package versions aligned with live packageGood job aligning the versions across packages:
- @hocuspocus/provider: ^2.15.0
- tiptap-markdown: ^0.8.10
- y-prosemirror: ^1.2.15
- yjs: ^13.6.20
Let's verify the tiptap-markdown update:
Also applies to: 68-68, 71-71, 73-73
✅ Verification successful
Package updates verified as safe
All package version updates are safe to use:
- @hocuspocus/provider ^2.15.0: Adds thread features and bug fixes
- y-prosemirror ^1.2.15: Contains cursor selection bug fixes
- yjs ^13.6.20: Adds
isDestroyedproperty and bug fixes🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check tiptap-markdown changelog for breaking changes echo "Checking tiptap-markdown changelog..." gh api repos/aguingand/tiptap-markdown/releases | jq -r ".[].body"Length of output: 2422
Script:
#!/bin/bash # Check @hocuspocus/provider changelog echo "Checking @hocuspocus/provider changelog..." gh api repos/ueberdosis/hocuspocus/releases | jq -r ".[].body" # Check y-prosemirror changelog echo "Checking y-prosemirror changelog..." gh api repos/yjs/y-prosemirror/releases | jq -r ".[].body" # Check yjs changelog echo "Checking yjs changelog..." gh api repos/yjs/yjs/releases | jq -r ".[].body"Length of output: 41975
Palanikannan1437
changed the title
Palanikannan1437
changed the title
Palanikannan1437
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/editor/src/core/types/editor.ts (1)
Line range hint
1-180: Consider refactoring the editor ref types for better performance.Given the PR's focus on editor performance and removing refs, consider these architectural improvements:
- Replace ref-based APIs with state-based or event-driven approaches
- Move real-time collaboration logic out of the editor core
- Consider using a pub/sub pattern for heading updates instead of callback-based approach
This would align better with the PR's objective of making the editor faster and removing refs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/types/editor.ts(1 hunks)
🔇 Additional comments (2)
packages/editor/src/core/types/editor.ts (2)
Line range hint
52-65: Verify the removal of real-time update methods.Based on the AI summary,
emitRealTimeUpdateandlistenToRealTimeUpdatemethods were removed fromEditorReadOnlyRefApi. This could impact real-time collaboration features in read-only mode.Let's check for any remaining usage of these methods:
104-104: Verify the migration of heading-related functionality.The
onHeadingChangecallback has been moved fromEditorReadOnlyRefApitoEditorRefApi. This suggests a change in the heading management strategy, potentially affecting existing code that relies on this functionality in read-only mode.Let's verify the impact of this change:
✅ Verification successful
Migration of
onHeadingChangeis correctly implementedThe move from
EditorReadOnlyRefApitoEditorRefApiis safe as there are no components using the read-only API, and the existing consumers likePageContentBrowseralready useEditorRefApi. The implementation maintains proper cleanup and event handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of heading-related methods in read-only contexts # Search for usage of getHeadings and onHeadingChange in read-only contexts rg -l "getHeadings|onHeadingChange" --type ts # Search for components using EditorReadOnlyRefApi ast-grep --pattern 'useRef<EditorReadOnlyRefApi>'Length of output: 293
Script:
#!/bin/bash # Check implementation details of heading functionality rg "onHeadingChange" -A 5 -B 5 --type ts # Check content browser implementation cat "web/core/components/pages/editor/summary/content-browser.tsx" # Check hook implementation cat "packages/editor/src/core/hooks/use-editor.ts"Length of output: 14920
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/editor/src/core/hooks/use-editor.ts (2)
145-148: Add error handling for content insertion.The content insertion logic should handle the case where the editor is null or destroyed.
Apply this improvement:
setEditorValueAtCursorPosition: (content: string) => { + if (!editor || editor.isDestroyed) { + console.error("Editor is not available or has been destroyed"); + return; + } if (editor.state.selection) { insertContentAtSavedSelection(editor, content); } },
269-271: Add type safety to document info retrieval.The optional chaining could be improved to handle all potential undefined cases.
Apply this improvement:
getDocumentInfo: () => ({ - characters: editor?.storage?.characterCount?.characters?.() ?? 0, - paragraphs: getParagraphCount(editor?.state), - words: editor?.storage?.characterCount?.words?.() ?? 0, + characters: editor?.storage?.characterCount?.characters?.() || 0, + paragraphs: editor?.state ? getParagraphCount(editor.state) : 0, + words: editor?.storage?.characterCount?.words?.() || 0, }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/hooks/use-editor.ts(10 hunks)
🔇 Additional comments (2)
packages/editor/src/core/hooks/use-editor.ts (2)
4-8: LGTM! Clean import statements.The import statements have been properly organized and renamed to avoid conflicts. Unused imports have been removed, improving code cleanliness.
77-78: Verify the impact of disabled rendering options.While disabling immediate rendering and transaction re-renders can improve performance, please verify that this doesn't cause issues with:
- Initial content display
- Real-time updates visibility
- Collaborative editing synchronization
✅ Verification successful
Disabled rendering options are safely implemented.
The editor maintains proper content synchronization and state management through explicit update handlers, making it safe to disable immediate rendering and transaction re-renders. The implementation includes:
- Proper content initialization
- Robust state synchronization
- Error handling for content updates
- Selection state preservation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related test files and rendering configurations echo "Searching for editor rendering tests..." rg -l "immediatelyRender|shouldRerenderOnTransaction" --type ts --type tsx echo "Searching for potential rendering issues in issues..." gh issue list --search "editor render" --json number,title,body --limit 10Length of output: 3109
Script:
#!/bin/bash # Search for editor initialization and transaction handling echo "Searching editor initialization..." ast-grep --pattern 'const useEditor = ($$$) => { $$$ }' echo "Searching transaction handling..." rg -A 5 "onTransaction" --type ts echo "Searching test files..." fd "test|spec" --extension ts --exec rg -l "editor.*render|transaction"Length of output: 25806
Description
Type of Change
Test Scenarios
Test all the editors accross platform
Summary by CodeRabbit
Summary by CodeRabbit
Dependencies
Editor Modifications
savedSelectionstate management.Type Changes